-
-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feat update schemas #803
Feat update schemas #803
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - good stuff 👍
I can't test it right now, but I left a few comments
hashtags: List[str] = None | ||
organisation_id: int = None | ||
organisation_logo: str = None | ||
title: Optional[str] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't gone through all of the docs for Pydantic V2, but I think the syntax might be:
title: Optional[str] = None
as Optional params still need a default value.
|
||
# Fetch the first row of the result | ||
db_organisation = result.fetchone() | ||
# Use SQLAlchemy's query-building capabilities |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try to avoid using SQLAlchemy, as we will remove it in the future.
The raw SQL equivalent is probably best 👍
result = db.execute(query) | ||
tasks = [task[0] for task in result.fetchall()] | ||
return tasks | ||
query = text(""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need the SQL wrapped in text
? That locks it in a bit more to SQLAlchemy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have used sqlalchemy db session. it was not required in the previous stage. But, it is required since your latest update. It might be because of pydantic??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No it's not related to pydantic, but there is no harm having the text()
there for now if it works.
We can refactor when we remove SQLAlchemy.
No description provided.